Skip to content

refactor: Split LoanInvariant into LoanBrokerInvariant and LoanInvariant#6674

Merged
bthomee merged 2 commits intodevelopfrom
tapanito/lending-invariants
Mar 27, 2026
Merged

refactor: Split LoanInvariant into LoanBrokerInvariant and LoanInvariant#6674
bthomee merged 2 commits intodevelopfrom
tapanito/lending-invariants

Conversation

@Tapanito
Copy link
Copy Markdown
Collaborator

Separate ValidLoanBroker and ValidLoan into their own header/source file pairs, following the existing pattern of other invariant files (e.g. AMMInvariant, VaultInvariant).

High Level Overview of Change

Context of Change

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

Separate ValidLoanBroker and ValidLoan into their own header/source
file pairs, following the existing pattern of other invariant files
(e.g. AMMInvariant, VaultInvariant).
@Tapanito Tapanito requested review from bthomee and godexsoft March 26, 2026 15:40
@Tapanito Tapanito added the Trivial Simple change with minimal effect, or already tested. Only needs one approval. label Mar 26, 2026
Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two unguarded at() calls in the invariant checker (lines 101, 115) can throw on malformed SLEs — see inline comments.

Review by Claude Opus 4.6 · Prompt: V12

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 91.25000% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.5%. Comparing base (509677a) to head (1776f1f).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines Patch % Lines
src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp 91.2% 7 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #6674   +/-   ##
=======================================
  Coverage     81.5%   81.5%           
=======================================
  Files          998     999    +1     
  Lines        74456   74456           
  Branches      7578    7555   -23     
=======================================
+ Hits         60648   60652    +4     
+ Misses       13808   13804    -4     
Files with missing lines Coverage Δ
include/xrpl/tx/invariants/InvariantCheck.h 100.0% <ø> (ø)
src/libxrpl/tx/invariants/LoanInvariant.cpp 63.6% <ø> (-19.5%) ⬇️
src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp 91.2% <91.2%> (ø)

... and 4 files with indirect coverage changes

Impacted file tree graph

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

}
if (after->at(sfDebtTotal) < 0)
{
JLOG(j.fatal()) << "Invariant failed: Loan Broker debt total is negative";
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tapanito Although a structural change only, as you told the AI bot, it would be a good idea to add:

  1. An LCOV exclusion, and
  2. An UNREACHABLE statement

here and below, if these places are not reached via unit tests / cannot be reached ever.

It's fine to do this in a follow-up PR, and probably should be part of a larger, comprehensive effort to do this properly across all invariants.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bthomee, I will do this as part of Lending Protocol invariants. They are a little bit scant at the moment.

@Tapanito Tapanito added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Mar 27, 2026
Copy link
Copy Markdown
Contributor

@xrplf-ai-reviewer xrplf-ai-reviewer bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One architectural concern flagged inline — keylet::unchecked inside an invariant checker lacks a rationale comment.

Review by Claude Opus 4.6 · Prompt: V12

if (indexes.size() == 1)
{
auto const index = indexes.value().front();
auto const sle = view.read(keylet::unchecked(index));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keylet::unchecked weakens invariant safety — directory entries can hold heterogeneous types, but add a comment explaining why unchecked is required here:

        // keylet::unchecked is intentional: the directory may hold either
        // ltRIPPLE_STATE or ltMPTOKEN entries; no single typed keylet covers
        // both.  The type is validated immediately below.
        auto const sle = view.read(keylet::unchecked(index));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Tapanito if this comment is valid, please also include it when you work on the larger Lending Protocol invariants effort.

@bthomee bthomee added this pull request to the merge queue Mar 27, 2026
Merged via the queue into develop with commit 9b944ee Mar 27, 2026
34 checks passed
@bthomee bthomee deleted the tapanito/lending-invariants branch March 27, 2026 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. Trivial Simple change with minimal effect, or already tested. Only needs one approval.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants